fix(ci): restore green main (workflow callers, type errors, coverage, hardening)#24
fix(ci): restore green main (workflow callers, type errors, coverage, hardening)#24williaby wants to merge 10 commits into
Conversation
Restores a green pipeline on a freshly generated project. The org reusable workflows had dropped several inputs/secrets the generated callers still passed, causing every run to fail at startup_failure (workflow-call validation). With CI never reaching the test step, real failures were masked. - ci.yml: drop removed inputs (enable-sonarcloud, sonarcloud-organization, sonarcloud-project-key, enable-codecov) and the secrets block no longer declared by python-ci.yml - security-analysis.yml: drop removed run-safety input - correlation.py: type call_next as Callable[[Request], Awaitable[Response]] so the awaited dispatch type-checks (fixes 2 BasedPyright errors) - security.py: guard server-header removal instead of MutableHeaders.pop - health.py: use Field(default=None, ...) so success-path constructors type-check (fixes 2 BasedPyright errors) - tests: remove dead TestCLI class (imported a non-existent cli module) and add unit tests for health and security middleware; coverage 55.7% -> 99.2% - docs: log template feedback for the stale callers and phantom CLI tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GitHub Actions hardening: - Pin astral-sh/setup-uv@v7 to v7.1.1 SHA across fips-compatibility, slsa-provenance, and pr-validation workflows. - Pin actions/dependency-review-action@v4 to v4.5.0 SHA. - Pin sonarsource/sonarqube-quality-gate-action@master (mutable) to v1.2.0 SHA. - Add step-security/harden-runner with egress-policy: audit to every local job that runs steps: ci-gate, codecov report-failure, both fips-compatibility jobs, dependency-review, both reuse jobs, both sonarcloud jobs, and pr-validation's validate-dependencies job. README: - Replace the placeholder "from foundry_unify import YourModule" example with a real FastAPI wiring example using CorrelationMiddleware, add_security_middleware, structured logging, and the health router. - Rewrite the Overview to describe what currently ships (FastAPI middleware, structured logging, Pydantic settings, exception hierarchy, K8s health probes) rather than the unimplemented OCR orchestrator. - Add a Configuration reference table for the FOUNDRY_UNIFY_* env vars exposed by core.config.Settings. - Update installation instructions to reflect that the package is not yet on PyPI (install from git for both pip and uv). Security audit notes: - No external HTTP requests, file I/O, eval/exec, subprocess, or deserialisation in the current source — only urllib.parse usage in SSRF middleware, which is read-only URL parsing. - SECURITY.md already lists byronawilliams@gmail.com and a GitHub Security Advisories link, so no change required. - Dependencies use lower-bound (>=) pins only; reproducibility is provided by uv.lock, so this is acceptable for an application. https://claude.ai/code/session_015Zx4CpQVNSvskJhJXT5v3q
The SonarCloud workflow uses setup-python@v5.3.0 with cache: 'pip', but the workflow installs UV via curl (not pip). The post-step fails: "Cache folder path is retrieved for pip but doesn't exist on disk: /home/runner/.cache/pip", which red-lights SonarCloud Analysis on every run despite SonarCloud itself succeeding. Removing the cache: 'pip' line eliminates the spurious failure; no pip-managed deps means no cache to preserve.
|
Warning Review limit reached
More reviews will be available in 59 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR hardens GitHub Actions CI workflows with runner security hardening and action pinning; documents accepted vulnerabilities via pip-audit configuration; substantially rewrites the README for Foundry Unify with installation and usage examples; refactors existing tests and adds comprehensive test coverage for health and security middleware components. ChangesCI/Workflow Hardening and Action Management
Vulnerability Management and Compliance
Documentation and Configuration Updates
Source Code Type Safety and Minor Fixes
Test Suite Refactoring and Coverage Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ FIPS Compatibility Check
Status: ✅ PASSED What is FIPS?FIPS 140-2/140-3 is a US government standard for cryptographic modules. Common issues:
|
There was a problem hiding this comment.
Pull request overview
This is a Wave 0 foundation PR aimed at making main green so that 20 other open PRs can rebase and merge. It removes inputs/secrets that the org's reusable workflows no longer accept (which caused startup_failure before any step ran), fixes the 5 BasedPyright errors that previously blocked the Code Quality gate, deletes a TestCLI class that imported a non-existent module, adds substantive unit tests for the health endpoints and security middleware, and folds in workflow hardening (harden-runner + SHA-pinned actions) and README rewrites consolidated from PRs #12, #14, #15, and #20.
Changes:
- Drop removed inputs/secrets (
enable-sonarcloud,enable-codecov,run-safety,cache: pip, etc.) from workflow callers and pin actions to SHAs with harden-runner steps. - Fix BasedPyright errors:
call_nexttyped asAwaitable[Response], guardeddel response.headers["server"],Field(default=None, ...)on optionalReadinessCheckfields, and aligned docstring with the_logger/_method_namesignature. - Remove the dead
TestCLIclass and add ~38 new unit tests forhealth.pyandsecurity.py, lifting coverage from 55.7% → 99.2%. Rewrite README to reflect what actually ships.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Drops removed inputs/secrets and adds harden-runner to ci-gate. |
.github/workflows/security-analysis.yml |
Drops removed run-safety input. |
.github/workflows/sonarcloud.yml |
Removes pip cache, pins quality-gate action, adds harden-runner. |
.github/workflows/codecov.yml |
Adds harden-runner step. |
.github/workflows/dependency-review.yml |
Pins action to v4.5.0 SHA, adds harden-runner. |
.github/workflows/fips-compatibility.yml |
Pins setup-uv SHA, adds harden-runner to both jobs. |
.github/workflows/pr-validation.yml |
Pins setup-uv SHA, adds harden-runner to validate-dependencies. |
.github/workflows/reuse.yml |
Adds harden-runner to both jobs. |
.github/workflows/slsa-provenance.yml |
Pins setup-uv to SHA. |
src/foundry_unify/api/health.py |
Uses Field(default=None, ...) so success-path constructors type-check. |
src/foundry_unify/middleware/correlation.py |
call_next typed as Awaitable[Response]; processor docstring aligned. |
src/foundry_unify/middleware/security.py |
Replaces unsupported MutableHeaders.pop with guarded del. |
tests/test_example.py |
Removes dead TestCLI class that imported a non-existent cli module. |
tests/unit/test_health.py |
New unit tests for health probes, helpers, and models. |
tests/unit/test_security_middleware.py |
New unit tests for security headers, rate limiting, and SSRF prevention. |
README.md |
Rewrites Overview, install, and usage sections to reflect what ships today. |
docs/template_feedback.md |
Logs phantom-CLI and stale-caller issues for the upstream template. |
| def test_readiness_503_logic_raises_http_exception(self) -> None: | ||
| """Verify that a failing check dict triggers the 503 HTTPException path.""" | ||
| checks = {"cache": ReadinessCheck(name="cache", status=False, error="down")} | ||
| all_healthy = all(check.status for check in checks.values()) | ||
|
|
||
| assert not all_healthy | ||
| with pytest.raises(HTTPException) as exc_info: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| detail={ | ||
| "status": "unavailable", | ||
| "checks": { | ||
| name: check.model_dump() for name, check in checks.items() | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| assert exc_info.value.status_code == 503 |
| def _make_app_with(*_middleware_classes_and_kwargs: Any) -> FastAPI: | ||
| """Build a minimal FastAPI app and register an echo route.""" | ||
| app = FastAPI() | ||
|
|
||
| @app.get("/ping") | ||
| async def ping() -> dict[str, str]: | ||
| return {"pong": "ok"} | ||
|
|
||
| return app | ||
|
|
|
|
||
| from __future__ import annotations | ||
|
|
||
| import asyncio |
Add a new annotation block covering 15 files that predated the centralized REUSE.toml coverage. These files were flagged by `reuse lint` (Check REUSE Compliance workflow) but had no matching path entry. Licensed CC0-1.0 to match the existing configuration-files annotation pattern in this repo's REUSE.toml. Unblocks the TruffleHog rollout PR #8 on this repo (which was failing the Check REUSE Compliance gate due to these pre-existing missing-info files, not because of the TruffleHog change). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Delete LICENSES/Apache-2.0.txt, LICENSES/BSD-3-Clause.txt, LICENSES/GPL-3.0-or-later.txt from LICENSES/. After the prior commit added coverage for previously-uncovered files, the only remaining REUSE compliance issue is unused license texts: no file in this repo is licensed under Apache-2.0, BSD-3-Clause, GPL-3.0-or-later. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The org reusable workflows default no-build:true, passing --no-build to uv sync. This repo uses the hatchling build backend, so the editable install must build a wheel; --no-build makes every run fail with "marked as --no-build but has no binary distribution". Set no-build:false on every caller that exposes the input (ci, security-analysis, python compatibility matrix, docs, mutation, sbom, release). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
✅ Mutation Testing Results
What is Mutation Testing?Mutation testing introduces small changes (mutations) to your code and checks if your tests detect them. A high mutation score indicates your tests are effective at catching bugs.
|
…trix Resolve the remaining real failures the revived pipeline surfaced: - deps: bump idna 3.13->3.17, pymdown-extensions 10.21.2->10.21.3, starlette 1.0.0->1.2.0, urllib3 2.6.3->2.7.0 to clear 5 pip-audit CVEs - pip-audit: ignore PYSEC-2022-42969 (py ReDoS in unused SVN path; transitive via interrogate, no fix exists) with a dated docs/known-vulnerabilities.md entry per project policy - docs: exclude template/scaffolding pages with placeholder links from the --strict mkdocs build (adr-template, project-plan-template, planning/adr) - ci: drop the -m marker from the compatibility test-command; its embedded quotes did not survive the org workflow shell, so pytest read "slow" as a path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/security-analysis.yml (1)
2-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale header comment after removing
run-safety.
run-safety: truewas dropped from the inputs, but the header still lists Safety scanning as a feature (lines 3 and 7). Update the comment so it reflects the actual scan set (Bandit, CodeQL, dependency review, OSV) and avoids misleading maintainers.📝 Suggested comment update
# Security Analysis Workflow -# Runs security scans including Bandit, Safety, and dependency checks. +# Runs security scans including Bandit, CodeQL, dependency review, and OSV. # # Features: # - Bandit static analysis for Python -# - Safety dependency vulnerability scanning +# - OSV / dependency-review vulnerability scanning # - Weekly scheduled scans🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/security-analysis.yml around lines 2 - 8, Update the stale header comment in the "Security Analysis Workflow" so it no longer lists Safety scanning; replace the features list to accurately reflect the actual scans run (Bandit static analysis, CodeQL, dependency-review, and OSV/dependency checks) and remove or adjust any mention of "Safety" and other outdated items; ensure the top comment block (the title and Features bullet points) mentions Bandit, CodeQL, dependency review, and OSV only, and that any scheduling note matches the workflow triggers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/dependency-review.yml:
- Around line 32-33: Update the checkout step that uses
"actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" to explicitly set
persist-credentials: false so the GITHUB_TOKEN is not written into .git/config;
locate the checkout step (the line with uses: actions/checkout...) in the
workflow and add the persist-credentials: false key under that step to disable
credential persistence for this read-only job.
In @.github/workflows/fips-compatibility.yml:
- Around line 62-63: Update both checkout steps that use the pinned
actions/checkout invocation (the one shown and the second at the checkout in the
fips-runtime-test job) to disable persisting the GITHUB_TOKEN by adding
persist-credentials: false to the checkout step inputs; specifically, in the
fips-check and fips-runtime-test jobs modify the checkout step that currently
reads "uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" to
include the persist-credentials: false key so the GITHUB_TOKEN is not written
into .git/config.
In @.github/workflows/python-compatibility.yml:
- Around line 46-49: The workflow currently runs pytest via the test-command
string and only excludes files by path (--ignore=tests/integration
--ignore=tests/load), which lets tests marked `@pytest.mark.integration` outside
tests/integration still run; either move all integration-marked tests into
tests/integration (so the existing --ignore covers them) or update the
test-command to explicitly deselect the marker (e.g., invoke pytest with -m "not
integration and not load") while ensuring the YAML quoting preserves the -m
argument (adjust the YAML string quoting or use a multiline/pipe scalar so the
embedded quotes survive); locate the test-command string in the workflow and
apply one of these two fixes.
In `@tests/unit/test_health.py`:
- Around line 142-159: The test currently raises an HTTPException directly
instead of exercising the readiness() endpoint; update the
test_readiness_503_logic_raises_http_exception to invoke the real readiness
logic (e.g., via the TestClient calling GET "/health/ready" or by importing the
health module and calling readiness()) and arrange for a failing check by
monkeypatching the readiness dependencies: import foundry_unify.api.health as
health_module, monkeypatch the function or variable that builds the checks (or
monkeypatch readiness() to return a failing checks dict) so that readiness()
runs its 503 branch, then assert the client response or raised HTTPException has
status_code 503; alternatively refactor readiness() to accept injectable checks
and pass a failing checks dict from the test.
- Around line 278-288: The test is brittle because patched_time relies on the
exact sequence of time.time() calls; instead, make the failure deterministic by
causing the cache operation used by check_cache to raise directly. Replace the
patched_time monkeypatch with a monkeypatch.setattr on the cache client method
invoked by check_cache (e.g., health_module.cache_client.get or
health_module.cache_client.ping) to raise RuntimeError("cache unavailable") for
the error-case test, and do the same for
test_check_external_service_error_on_exception by monkeypatching the external
service call it relies on; leave time.time untouched so any extra timing reads
won't break the test.
- Around line 433-434: Remove the unused asyncio import and the dummy assignment
that suppresses the linter warning: delete the top-level import of asyncio and
the line assigning it to `_` (the placeholder used to avoid an unused-import
error) so tests rely on pytest.mark.asyncio only and no longer include the
unused asyncio symbol.
In `@tests/unit/test_security_middleware.py`:
- Around line 31-39: The helper function _make_app_with currently ignores its
parameters and isn't used; either remove the unused function or implement it to
apply provided middleware to the FastAPI app: update _make_app_with to accept
middleware classes/kwargs and register them on the FastAPI instance (use
app.add_middleware or include them when constructing FastAPI) so tests can pass
middleware into _make_app_with, or delete the function and update tests to
construct apps directly; refer to the _make_app_with function name and the ping
route inside for locating the code to change.
---
Outside diff comments:
In @.github/workflows/security-analysis.yml:
- Around line 2-8: Update the stale header comment in the "Security Analysis
Workflow" so it no longer lists Safety scanning; replace the features list to
accurately reflect the actual scans run (Bandit static analysis, CodeQL,
dependency-review, and OSV/dependency checks) and remove or adjust any mention
of "Safety" and other outdated items; ensure the top comment block (the title
and Features bullet points) mentions Bandit, CodeQL, dependency review, and OSV
only, and that any scheduling note matches the workflow triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d039dd9-2239-4c82-b9e2-290d3838b2f2
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (29)
.github/workflows/ci.yml.github/workflows/codecov.yml.github/workflows/dependency-review.yml.github/workflows/docs.yml.github/workflows/fips-compatibility.yml.github/workflows/mutation-testing.yml.github/workflows/pr-validation.yml.github/workflows/python-compatibility.yml.github/workflows/release.yml.github/workflows/reuse.yml.github/workflows/sbom.yml.github/workflows/security-analysis.yml.github/workflows/slsa-provenance.yml.github/workflows/sonarcloud.ymlLICENSES/Apache-2.0.txtLICENSES/BSD-3-Clause.txtLICENSES/GPL-3.0-or-later.txtREADME.mdREUSE.tomldocs/known-vulnerabilities.mddocs/template_feedback.mdmkdocs.ymlpyproject.tomlsrc/foundry_unify/api/health.pysrc/foundry_unify/middleware/correlation.pysrc/foundry_unify/middleware/security.pytests/test_example.pytests/unit/test_health.pytests/unit/test_security_middleware.py
💤 Files with no reviewable changes (4)
- LICENSES/Apache-2.0.txt
- LICENSES/GPL-3.0-or-later.txt
- LICENSES/BSD-3-Clause.txt
- tests/test_example.py
| - name: Checkout repository | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Set persist-credentials: false on checkout.
This job only reads the dependency diff and never pushes, so the GITHUB_TOKEN does not need to persist in .git/config. Disabling persistence reduces the credential-exfiltration surface for the subsequently invoked third-party action.
🛡️ Proposed hardening
- name: Checkout repository
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: falseAs per coding guidelines for .github/workflows/**: review for "Security best practices (minimal permissions, pinned actions)" and "Proper secret handling".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout repository | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| - name: Checkout repository | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 32-33: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/dependency-review.yml around lines 32 - 33, Update the
checkout step that uses
"actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" to explicitly set
persist-credentials: false so the GITHUB_TOKEN is not written into .git/config;
locate the checkout step (the line with uses: actions/checkout...) in the
workflow and add the persist-credentials: false key under that step to disable
credential persistence for this read-only job.
| - name: Checkout repository | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Set persist-credentials: false on checkout (applies to both jobs).
Neither fips-check nor fips-runtime-test pushes to the repo, but both execute repository scripts and a github-script step after checkout. Persisting the GITHUB_TOKEN in .git/config is unnecessary and widens the exfiltration surface. The same applies to the checkout in fips-runtime-test at Line 213-214.
🛡️ Proposed hardening (apply to both checkout steps)
- name: Checkout repository
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: falseAs per coding guidelines for .github/workflows/**: review for "Security best practices (minimal permissions, pinned actions)" and "Proper secret handling".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout repository | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| - name: Checkout repository | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 62-63: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/fips-compatibility.yml around lines 62 - 63, Update both
checkout steps that use the pinned actions/checkout invocation (the one shown
and the second at the checkout in the fips-runtime-test job) to disable
persisting the GITHUB_TOKEN by adding persist-credentials: false to the checkout
step inputs; specifically, in the fips-check and fips-runtime-test jobs modify
the checkout step that currently reads "uses:
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" to include the
persist-credentials: false key so the GITHUB_TOKEN is not written into
.git/config.
| # No -m marker expression: the embedded quotes do not survive the org | ||
| # workflow's shell expansion (pytest then reads "slow" as a path). The | ||
| # integration/load suites are excluded by path instead. | ||
| test-command: 'pytest tests/ -v --tb=short -x --ignore=tests/integration --ignore=tests/load' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tests marked slow that are outside tests/integration and tests/load.
rg -nP '`@pytest`\.mark\.slow' -g 'tests/**/*.py' \
-g '!tests/integration/**' -g '!tests/load/**'Repository: ByronWilliamsCPA/Unify
Length of output: 48
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the workflow around the cited lines and also any prior `-m` usage in the file
echo "---- workflow snippet (.github/workflows/python-compatibility.yml) ----"
nl -ba .github/workflows/python-compatibility.yml | sed -n '1,120p'
# 2) Search for any `-m` expressions mentioning slow/integration in this workflow
echo "---- -m expressions in workflow ----"
rg -n "pytest.*-m|--ignore=tests/integration|--ignore=tests/load|slow|integration" .github/workflows/python-compatibility.yml
# 3) Find slow-marked tests anywhere
echo "---- `@pytest.mark.slow` anywhere under tests ----"
rg -nP '`@pytest`\.mark\.slow' tests/**/*.py || true
# 4) Re-run the original "outside integration/load" query (the earlier run returned exit 1)
echo "---- `@pytest.mark.slow` outside tests/integration and tests/load ----"
rg -nP '`@pytest`\.mark\.slow' -g 'tests/**/*.py' \
-g '!tests/integration/**' -g '!tests/load/**' || trueRepository: ByronWilliamsCPA/Unify
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- workflow file head (with line numbers) ----"
cat -n .github/workflows/python-compatibility.yml | sed -n '1,120p'
echo "---- -m / slow / ignore usage in workflow ----"
rg -n "pytest.*-m|--ignore=tests/integration|--ignore=tests/load|slow|integration" .github/workflows/python-compatibility.yml || true
echo "---- `@pytest.mark.slow` anywhere under tests ----"
rg -nP '`@pytest`\.mark\.slow' tests/**/*.py || true
echo "---- `@pytest.mark.slow` outside integration/load dirs ----"
rg -nP '`@pytest`\.mark\.slow' -g 'tests/**/*.py' \
-g '!tests/integration/**' -g '!tests/load/**' || trueRepository: ByronWilliamsCPA/Unify
Length of output: 2545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- any pytest -m usage mentioning slow/integration in workflows ----"
rg -n "pytest .* -m|pytest.*--ignore=tests/integration|--ignore=tests/load|not slow|slow and not|integration and not" .github/workflows || true
echo "---- references to 'slow' in tests (broader than mark.slow) ----"
rg -n "slow" tests || true
echo "---- pytest marker declarations for slow (pytest.ini/pyproject/etc.) ----"
rg -n "markers:|@?pytest\.mark|slow" pyproject.toml tests . || trueRepository: ByronWilliamsCPA/Unify
Length of output: 26934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- any pytest.mark.slow / mark.slow usage ----"
rg -nP 'pytest\.mark\.slow|mark\.slow' tests/**/*.py || true
echo "---- any -m selections referencing slow in workflows ----"
rg -n "pytest .* -m|pytest.*--ignore|not slow|not slow and|slow and" .github/workflows || true
echo "---- any 'slow' marker selection in repo docs/config (for context) ----"
rg -n "not slow|mark.slow|`@pytest`\.mark\.slow" pyproject.toml tests .github/workflows || trueRepository: ByronWilliamsCPA/Unify
Length of output: 563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- `@pytest.mark.integration` anywhere ----"
rg -nP '`@pytest`\.mark\.integration|pytestmark\s*=\s*pytest\.mark\.integration' tests/**/*.py || true
echo "---- `@pytest.mark.integration` outside tests/integration dir ----"
rg -nP '`@pytest`\.mark\.integration|pytestmark\s*=\s*pytest\.mark\.integration' \
-g 'tests/**/*.py' \
-g '!tests/integration/**' \
|| trueRepository: ByronWilliamsCPA/Unify
Length of output: 386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- tests/conftest.py (relevant range) ----"
cat -n tests/conftest.py | sed -n '1,140p'
echo "---- any code that programmatically applies the 'slow' mark ----"
rg -n "add_marker\\(['\"]slow['\"]\\)|mark\\.slow|pytest\\.mark\\.slow|\\bslow\\b.*mark|mark.*\\bslow\\b" tests/**/*.py tests/conftest.py pyproject.toml || trueRepository: ByronWilliamsCPA/Unify
Length of output: 5226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- all occurrences of 'slow' in tests/conftest.py ----"
rg -n "slow" tests/conftest.py || true
echo "---- pytest collection hooks in tests/conftest.py ----"
rg -n "pytest_collection_|add_marker|collect|modifyitems" tests/conftest.py || trueRepository: ByronWilliamsCPA/Unify
Length of output: 296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tests/test_example.py | sed -n '150,240p'Repository: ByronWilliamsCPA/Unify
Length of output: 2455
Fix test selection semantics: --ignore doesn’t replace marker deselection for integration
# No -m marker expression: the embedded quotes do not survive the org
# workflow's shell expansion (pytest then reads "slow" as a path). The
# integration/load suites are excluded by path instead.
test-command: 'pytest tests/ -v --tb=short -x --ignore=tests/integration --ignore=tests/load'
The workflow no longer uses -m marker filtering, but the repo contains no @pytest.mark.slow tests, so dropping the slow deselection isn’t a concern here. The real change is that integration-marked tests outside tests/integration will now run (e.g., tests/test_example.py), so align the desired selection by either moving those tests under tests/integration or excluding them by path as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/python-compatibility.yml around lines 46 - 49, The
workflow currently runs pytest via the test-command string and only excludes
files by path (--ignore=tests/integration --ignore=tests/load), which lets tests
marked `@pytest.mark.integration` outside tests/integration still run; either move
all integration-marked tests into tests/integration (so the existing --ignore
covers them) or update the test-command to explicitly deselect the marker (e.g.,
invoke pytest with -m "not integration and not load") while ensuring the YAML
quoting preserves the -m argument (adjust the YAML string quoting or use a
multiline/pipe scalar so the embedded quotes survive); locate the test-command
string in the workflow and apply one of these two fixes.
| def test_readiness_503_logic_raises_http_exception(self) -> None: | ||
| """Verify that a failing check dict triggers the 503 HTTPException path.""" | ||
| checks = {"cache": ReadinessCheck(name="cache", status=False, error="down")} | ||
| all_healthy = all(check.status for check in checks.values()) | ||
|
|
||
| assert not all_healthy | ||
| with pytest.raises(HTTPException) as exc_info: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| detail={ | ||
| "status": "unavailable", | ||
| "checks": { | ||
| name: check.model_dump() for name, check in checks.items() | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| assert exc_info.value.status_code == 503 |
There was a problem hiding this comment.
This test does not exercise the readiness 503 path; it asserts a tautology.
The test constructs checks locally and then manually raises an HTTPException inside pytest.raises, so it only verifies that raising an exception raises that exception. The actual 503 branch in readiness() is never invoked (the real checks dict is always empty because the cache/external checks are commented out), so this test would not catch a regression in the endpoint's failure logic despite contributing to the coverage number.
Consider driving the real endpoint, e.g. by monkeypatching the checks into readiness() so the 503 path is genuinely executed:
💡 Sketch of a behavior-driven approach
def test_readiness_returns_503_when_check_fails(
self, client: TestClient, monkeypatch: pytest.MonkeyPatch
) -> None:
import foundry_unify.api.health as health_module
async def failing_readiness() -> None:
raise HTTPException(status_code=503, detail={"status": "unavailable"})
# or refactor readiness() to make `checks` injectable/overridable,
# then assert client.get("/health/ready").status_code == 503Note that this likely requires making the dependency checks injectable in readiness() first.
As per coding guidelines: "Verify that tests verify behavior, not just execution" and "Verify that tests would catch regressions in the feature being tested".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_health.py` around lines 142 - 159, The test currently raises
an HTTPException directly instead of exercising the readiness() endpoint; update
the test_readiness_503_logic_raises_http_exception to invoke the real readiness
logic (e.g., via the TestClient calling GET "/health/ready" or by importing the
health module and calling readiness()) and arrange for a failing check by
monkeypatching the readiness dependencies: import foundry_unify.api.health as
health_module, monkeypatch the function or variable that builds the checks (or
monkeypatch readiness() to return a failing checks dict) so that readiness()
runs its 503 branch, then assert the client response or raised HTTPException has
status_code 503; alternatively refactor readiness() to accept injectable checks
and pass a failing checks dict from the test.
| def patched_time() -> float: | ||
| nonlocal call_count | ||
| call_count += 1 | ||
| # Call 1: start = time.time() in check_cache - succeeds | ||
| # Call 2: latency calculation inside try block - raises | ||
| # Call 3: latency calculation inside except block - succeeds | ||
| if call_count == 2: | ||
| raise RuntimeError("cache unavailable") | ||
| return real_time() | ||
|
|
||
| monkeypatch.setattr(health_module.time, "time", patched_time) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Brittle coupling to the exact time.time() call sequence.
The patched time relies on the exception firing on precisely the 2nd time.time() invocation inside check_cache. Any change to the implementation (e.g., an added log timestamp or an extra timing read) silently shifts the call count and makes this test pass/fail for the wrong reason. The same pattern is repeated in test_check_external_service_error_on_exception (lines 348-353). Consider forcing the failure through the operation under test rather than a positional time.time() count.
As per coding guidelines: "No flaky tests or timing dependencies".
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 285-285: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_health.py` around lines 278 - 288, The test is brittle
because patched_time relies on the exact sequence of time.time() calls; instead,
make the failure deterministic by causing the cache operation used by
check_cache to raise directly. Replace the patched_time monkeypatch with a
monkeypatch.setattr on the cache client method invoked by check_cache (e.g.,
health_module.cache_client.get or health_module.cache_client.ping) to raise
RuntimeError("cache unavailable") for the error-case test, and do the same for
test_check_external_service_error_on_exception by monkeypatching the external
service call it relies on; leave time.time untouched so any extra timing reads
won't break the test.
| # Keep asyncio imported to suppress unused import warning from ruff | ||
| _ = asyncio |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove the unused asyncio import instead of suppressing it.
asyncio is imported (line 9) only to be assigned to _ here. Drop both the import and this line; the async tests use pytest.mark.asyncio and do not reference asyncio directly.
♻️ Proposed cleanup
-import asyncio
-
import pytest-
-# Keep asyncio imported to suppress unused import warning from ruff
-_ = asyncio🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_health.py` around lines 433 - 434, Remove the unused asyncio
import and the dummy assignment that suppresses the linter warning: delete the
top-level import of asyncio and the line assigning it to `_` (the placeholder
used to avoid an unused-import error) so tests rely on pytest.mark.asyncio only
and no longer include the unused asyncio symbol.
| def _make_app_with(*_middleware_classes_and_kwargs: Any) -> FastAPI: | ||
| """Build a minimal FastAPI app and register an echo route.""" | ||
| app = FastAPI() | ||
|
|
||
| @app.get("/ping") | ||
| async def ping() -> dict[str, str]: | ||
| return {"pong": "ok"} | ||
|
|
||
| return app |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm _make_app_with is never referenced outside its definition
rg -nP '\b_make_app_with\s*\(' --type=pyRepository: ByronWilliamsCPA/Unify
Length of output: 177
Remove unused _make_app_with helper
tests/unit/test_security_middleware.py defines _make_app_with(*_middleware_classes_and_kwargs) but never uses its arguments, and there are no references to _make_app_with elsewhere (only its definition exists). Remove it or repurpose it to actually build the app with the provided middleware.
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 31-31: Dynamically typed expressions (typing.Any) are disallowed in *_middleware_classes_and_kwargs
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_security_middleware.py` around lines 31 - 39, The helper
function _make_app_with currently ignores its parameters and isn't used; either
remove the unused function or implement it to apply provided middleware to the
FastAPI app: update _make_app_with to accept middleware classes/kwargs and
register them on the FastAPI instance (use app.add_middleware or include them
when constructing FastAPI) so tests can pass middleware into _make_app_with, or
delete the function and update tests to construct apps directly; refer to the
_make_app_with function name and the ping route inside for locating the code to
change.
The container build failed at uv sync because .dockerignore excluded README.md, which the hatchling backend requires (pyproject declares readme = "README.md"). Stop ignoring it. Also document the DL3008 hadolint exception inline: pinning Debian apt versions breaks builds once those versions age out of the repos. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The DL3008 ignore had explanatory comment lines between the directive and the RUN instruction, so hadolint did not associate it. Move the "# hadolint ignore=DL3008" line to sit directly above each apt-get RUN. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
debian-slim and distroless both ship CRITICAL/HIGH base-OS CVEs (perl-base Archive::Tar, curl/libssh2, ncurses, zlib, sqlite) with no fixes available, failing the Trivy gate. Switch both stages to Chainguard's Wolfi-based python images, which carry a near-zero CVE surface. - builder: chainguard python:latest-dev, digest-pinned (Renovate-updatable) - runtime: chainguard python:latest, digest-pinned; runs as nonroot (65532) - force uv to use the image python (UV_PYTHON, no downloads) so the venv interpreter path stays valid across stages - replace the curl HEALTHCHECK with a python urllib check (no curl/shell in the runtime) and run uvicorn via `python -m` Verified locally: image builds, Trivy reports 0 CRITICAL/HIGH, hadolint clean. BREAKING CHANGE: runtime base image changed from python:3.12-slim to Chainguard python (3.14); the image is now shell-less and distroless-style. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Context
mainwas red and all 20 open PRs were deadlocked behind it. The org reusable workflows had removed several inputs/secrets that the generated caller workflows still passed, so every run failed atstartup_failure(workflow-call validation) before any step executed. With CI never reaching the test phase, real failures stayed hidden.This is the Wave 0 foundation PR: it makes
maingenuinely green so the remaining PRs can be rebased and merged. It consolidates the overlapping fixes from #12, #14, #15, and #20.Root causes fixed
enable-sonarcloud,sonarcloud-organization,sonarcloud-project-key,enable-codecov) and thesecrets:block thatpython-ci.ymlno longer declares.run-safetyinput.call_nextasCallable[[Request], Awaitable[Response]](fixes 2 BasedPyright errors); align processor docstring with the_logger/_method_namesignature.MutableHeaders.pop(1 error).Field(default=None, ...)so success-path constructors type-check (2 errors).Test suite
TestCLIclass (imported a non-existentclimodule; no[project.scripts]entry exists).Folded-in / consolidated PRs
setup-pythonpip cache from SonarCloud workflow - cherry-picked.After merge, #12, #14, #15, #20 will be closed as consolidated.
Template feedback
Logged the stale-caller and phantom-CLI-test issues in
docs/template_feedback.mdfor upstream.Generated with Claude Code
Summary by CodeRabbit
Documentation
uvandpip, and expanded security middleware configuration guidanceConfiguration
Security